Improve error model and serialization#44
Improve error model and serialization#44Quinn-With-Two-Ns wants to merge 2 commits intonexus-rpc:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b7fcca303
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
3b7fcca to
0d96ba5
Compare
| * @param causeMessage The cause message. | ||
| * @deprecated | ||
| */ | ||
| public HandlerException(ErrorType errorType, String causeMessage) { |
There was a problem hiding this comment.
Seems like we should have a non deprecated constructor that will take just error type and message but I understand that this would break compatibility. In Go we managed to get around this by creating a new constructor but in Java I'm not sure how we might achieve that. Do you think we could remove this deprecated method and bring it back at some point?
There was a problem hiding this comment.
Yes I think we would remove this constructor in the future, I will add a note of this in the deprecated comment
| * @param message The cancellation message. | ||
| * @return The operation exception. | ||
| */ | ||
| public static OperationException canceled(String message) { |
There was a problem hiding this comment.
Noticed we are inconsistent with the naming. I would choose failed and canceled over failure and canceled.
There was a problem hiding this comment.
I agree they are inconsistent, is the suggestion to deprecate the old failure constructor? We can't remove it due to backwards compatibility
Improve error model and serialization:
OperationExceptionto have a messageHandlerExceptionto have a messageHandlerException